-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IND-485]: Query for PerpetualPositions by openEventId #777
Conversation
IND-485 Inconsistent perpetual position querying
It seems like two perpetual positions of the same height can be created. We should use |
# Walkthrough
The changes primarily revolve around the introduction of a new constant `defaultTendermintEvent4` and modifications to the sorting order in various functions and SQL queries. The sorting order has been changed from `createdAtHeight` to `openEventId` in the `findAll` function and in several SQL scripts. Additionally, new test cases have been added to verify the correct sorting of `PerpetualPositions` based on `openEventId`.
# Changes
| File Path | Change Summary |
| --- | --- |
| `.../postgres/__tests__/helpers/constants.ts`<br>`.../postgres/__tests__/helpers/mock-generators.ts`<br>`.../postgres/__tests__/stores/perpetual-position-table.test.ts` | Addition of a new constant `defaultTendermintEvent4` of type `TendermintEventCreateObject` and `defaultTendermintEventId4` of type `Buffer`. Corresponding creation and usage in test data and functions. |
| `.../postgres/src/stores/perpetual-position-table.ts`<br>`.../ender/src/scripts/dydx_order_fill_handler_per_order.sql`<br>`.../ender/src/scripts/dydx_update_perpetual_position_aggregate_fields.sql` | Modification of sorting order from `createdAtHeight` to `openEventId` in the `findAll` function and SQL queries. |
| `.../ender/__tests__/handlers/order-fills/liquidation-handler.test.ts`<br>`.../ender/__tests__/handlers/order-fills/order-handler.test.ts` | Changes to event IDs and test constants. | TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/helpers/mock-generators.ts (2 hunks)
- indexer/packages/postgres/tests/stores/perpetual-position-table.test.ts (3 hunks)
- indexer/packages/postgres/src/stores/perpetual-position-table.ts (1 hunks)
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (4 hunks)
- indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/postgres/tests/helpers/constants.ts
- indexer/packages/postgres/tests/helpers/mock-generators.ts
Additional comments: 11
indexer/packages/postgres/src/stores/perpetual-position-table.ts (1)
- 124-129: The ordering of the results from the
findAll
function has been changed fromcreatedAtHeight
toopenEventId
. Ensure that this change does not affect any dependent functions or modules that rely on the previous ordering.indexer/services/ender/src/scripts/dydx_order_fill_handler_per_order.sql (1)
- 167-173: The change in the ORDER BY clause from "createdAtHeight" to "openEventId" may affect the order of the records retrieved from the database. Ensure that this change does not introduce any unintended side effects in the application logic that relies on the order of these records.
- ORDER BY "createdAtHeight" DESC; + ORDER BY "openEventId" DESC;indexer/services/ender/__tests__/handlers/order-fills/liquidation-handler.test.ts (3)
144-145: Ensure that the
openEventId
andlastEventId
values are correctly set and that they are consistent with the new ordering logic.237-237: The message 'creates fills and orders (with %s), sends vulcan message for maker order update and updates perpetualPosition' is clear and descriptive. Ensure that it accurately reflects the test case.
300-300: Ensure that the
openEventId
value is correctly set and that it is consistent with the new ordering logic.indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (4)
146-147: Ensure that the
openEventId
andlastEventId
are correctly updated in all relevant places in the codebase.314-318: Ensure that the
openEventId
is correctly updated in all relevant places in the codebase.917-920: Ensure that the
openEventId
is correctly updated in all relevant places in the codebase.1129-1140: Ensure that the
openEventId
is correctly updated in all relevant places in the codebase.indexer/packages/postgres/__tests__/stores/perpetual-position-table.test.ts (2)
15-18: The new import
TendermintEventCreateObject
is used in the new test case. Ensure that this import is used correctly and that the type is defined as expected.191-224: This new test case checks if the
PerpetualPositions
are sorted byopenEventId
. It creates two positions with differentopenEventId
s and checks if the returned positions are sorted correctly. The test case seems to be correct, but ensure that thePerpetualPositionTable.findAll
function is implemented to sort byopenEventId
.+ it('Successfully finds PerpetualPositions sorted by openEventId', async () => { + const earlierPosition: PerpetualPositionCreateObject = { + ...defaultPerpetualPosition, + openEventId: defaultTendermintEventId3, + lastEventId: defaultTendermintEventId3, + }; + const nextTendermintEvent: TendermintEventCreateObject = { + blockHeight: defaultTendermintEvent3.blockHeight, + transactionIndex: defaultTendermintEvent3.transactionIndex, + eventIndex: defaultTendermintEvent3.eventIndex + 1, + }; + const nextTendermintEventId: Buffer = TendermintEventTable.createEventId( + nextTendermintEvent.blockHeight, + nextTendermintEvent.transactionIndex, + nextTendermintEvent.eventIndex, + ); + const laterPosition: PerpetualPositionCreateObject = { + ...defaultPerpetualPosition, + openEventId: nextTendermintEventId, + lastEventId: nextTendermintEventId, + }; + await TendermintEventTable.create(nextTendermintEvent); + await Promise.all([ + await PerpetualPositionTable.create(earlierPosition), + await PerpetualPositionTable.create(laterPosition), + ]); + + const perpetualPositions: PerpetualPositionFromDatabase[] = await + PerpetualPositionTable.findAll({}, [], { readReplica: true }); + + expect(perpetualPositions.length).toEqual(2); + expect(perpetualPositions[0]).toEqual(expect.objectContaining(laterPosition)); + expect(perpetualPositions[1]).toEqual(expect.objectContaining(earlierPosition)); + });
910858c
to
a564b30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (7)
- indexer/packages/postgres/tests/helpers/constants.ts (2 hunks)
- indexer/packages/postgres/tests/helpers/mock-generators.ts (2 hunks)
- indexer/packages/postgres/tests/stores/perpetual-position-table.test.ts (3 hunks)
- indexer/packages/postgres/src/stores/perpetual-position-table.ts (1 hunks)
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts (3 hunks)
- indexer/services/ender/tests/handlers/order-fills/order-handler.test.ts (4 hunks)
- indexer/services/ender/src/scripts/dydx_update_perpetual_position_aggregate_fields.sql (1 hunks)
Files skipped from review due to trivial changes (2)
- indexer/packages/postgres/tests/helpers/mock-generators.ts
- indexer/services/ender/tests/handlers/order-fills/liquidation-handler.test.ts
Additional comments: 9
indexer/services/ender/src/scripts/dydx_update_perpetual_position_aggregate_fields.sql (1)
- 22-30: The change in ordering from "createdAtHeight" to "openEventId" could potentially alter the result of the query. Ensure that this change is intended and that it does not break any dependent functionality. Also, verify that "openEventId" is always unique and non-null to prevent unexpected results.
indexer/packages/postgres/__tests__/stores/perpetual-position-table.test.ts (2)
15-18: The new import
TendermintEventCreateObject
is used in the new test case for sortingPerpetualPositions
byopenEventId
. This is in line with the changes made in the PR.191-224: This new test case checks if the
PerpetualPositions
are sorted byopenEventId
. It creates twoPerpetualPositions
with differentopenEventId
s and checks if the returned positions are sorted correctly. This is a good addition to the test suite as it verifies the new functionality introduced in this PR.indexer/services/ender/__tests__/handlers/order-fills/order-handler.test.ts (3)
146-147: Ensure that the new
openEventId
andlastEventId
values are correct and that they are being used correctly in the tests.317-318: Ensure that the
openEventId
value is correct and that it is being used correctly in the tests.1140-1140: Ensure that the
openEventId
value is correct and that it is being used correctly in the tests.indexer/packages/postgres/__tests__/helpers/constants.ts (2)
287-294: The addition of
defaultTendermintEvent4
is consistent with the existing pattern of defining test constants. Ensure that the values assigned toblockHeight
,transactionIndex
, andeventIndex
are appropriate for the tests where this constant will be used.310-314: The creation of
defaultTendermintEventId4
is consistent with the existing pattern of defining test constants. Ensure that the values passed tocreateEventId
are appropriate for the tests where this constant will be used.indexer/packages/postgres/src/stores/perpetual-position-table.ts (1)
- 124-130: The ordering of the query results has been changed from
createdAtHeight
toopenEventId
. Ensure that this change does not affect any dependent logic that relies on the previous ordering.- baseQuery = baseQuery.orderBy( - PerpetualPositionColumns.createdAtHeight, - Ordering.ASC, - ); + baseQuery = baseQuery.orderBy( + PerpetualPositionColumns.subaccountId, + Ordering.ASC, + ).orderBy( + PerpetualPositionColumns.openEventId, + Ordering.DESC, + );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql (1 hunks)
Files skipped from review due to trivial changes (1)
- indexer/services/ender/src/scripts/dydx_liquidation_fill_handler_per_order.sql
Changelist
Change existing queries to
perpetual_positions
to order byopenEventId
rather thancreatedAtHeight
and change the corresponding SQL statement in Ender to also useopentEventId
Test Plan
Unit tests
Author/Reviewer Checklist
state-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.